Skip to content

Migrate jobs to new user model#5447

Merged
tobiasKaminsky merged 2 commits into
masterfrom
ezaquarii/migrate-jobs-to-new-user-model
Feb 17, 2020
Merged

Migrate jobs to new user model#5447
tobiasKaminsky merged 2 commits into
masterfrom
ezaquarii/migrate-jobs-to-new-user-model

Conversation

@ezaquarii

@ezaquarii ezaquarii commented Feb 10, 2020

Copy link
Copy Markdown
Collaborator

Jobs are migrated to new user model.
Some of the APIs that jobs use are not yet user-ready - we use toPlatformAccount. As ususal, the plan is to scoop those cases when we refactor those dependencies.

There should be no changes in logic - this is pretty mechanical change.

@ezaquarii
ezaquarii force-pushed the ezaquarii/migrate-jobs-to-new-user-model branch from 9301750 to 0e0be92 Compare February 10, 2020 22:05
@ezaquarii ezaquarii changed the title Ezaquarii/migrate jobs to new user model [WIP] Migrate jobs to new user model Feb 10, 2020
@codecov

codecov Bot commented Feb 10, 2020

Copy link
Copy Markdown

Codecov Report

Merging #5447 into master will decrease coverage by <.01%.
The diff coverage is 1.56%.

@@             Coverage Diff              @@
##             master    #5447      +/-   ##
============================================
- Coverage     20.93%   20.92%   -0.01%     
  Complexity        3        3              
============================================
  Files           398      398              
  Lines         33324    33353      +29     
  Branches       4674     4678       +4     
============================================
+ Hits           6975     6978       +3     
- Misses        25219    25245      +26     
  Partials       1130     1130
Impacted Files Coverage Δ Complexity Δ
src/main/java/com/owncloud/android/MainApp.java 48.95% <ø> (ø) 0 <0> (ø) ⬇️
...m/nextcloud/client/account/UserAccountManager.java 0% <ø> (ø) 0 <0> (ø) ⬇️
...java/com/owncloud/android/jobs/OfflineSyncJob.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ndroid/ui/activity/ContactsPreferenceActivity.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...ava/com/owncloud/android/jobs/NotificationJob.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...a/com/owncloud/android/jobs/ContactsBackupJob.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...n/java/com/owncloud/android/jobs/FilesSyncJob.java 0% <0%> (ø) 0 <0> (ø) ⬇️
...xtcloud/client/account/UserAccountManagerImpl.java 60.68% <0%> (-1.73%) 0 <0> (ø)
...wncloud/android/jobs/MediaFoldersDetectionJob.java 23.27% <0%> (-1.5%) 0 <0> (ø)
...a/com/owncloud/android/jobs/AccountRemovalJob.java 0% <0%> (ø) 0 <0> (ø) ⬇️
... and 8 more

Signed-off-by: Chris Narkiewicz <hello@ezaquarii.com>
@ezaquarii
ezaquarii force-pushed the ezaquarii/migrate-jobs-to-new-user-model branch from 0e0be92 to 0afc264 Compare February 14, 2020 06:22
@ezaquarii ezaquarii changed the title [WIP] Migrate jobs to new user model Migrate jobs to new user model Feb 14, 2020
@nextcloud nextcloud deleted a comment from ezaquarii Feb 14, 2020
@nextcloud nextcloud deleted a comment from ezaquarii Feb 14, 2020
@nextcloud nextcloud deleted a comment from ezaquarii Feb 14, 2020
private final UploadsStorageManager uploadsStorageManager;
private final UserAccountManager userAccountManager;
private final Clock clock;
private final EventBus eventBus;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that this was used before, so I am totally fine to re-use this.
But as our approach as to get rid of 3rd party libs, we should think about removing EventBus and evernote Job scheduling, but rely on WorkManager and
[https://developer.android.com/topic/libraries/architecture/workmanager/how-to/states-and-observation#observing](observing your work)

But let us do this in a following PR, please :-)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the evernote thing, but why is it that we hate eventbus these days? It's a relatively established and long-living project that won't disappear tomorrow.

@ezaquarii ezaquarii Feb 14, 2020

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tobiasKaminsky I didn't add it - I just extracted a dependency that was explicitly created in a job constructor.

@mario Good question!

EventBus is not a bad piece of engineering in itself. However, how it's being used in practice is another story and mileage will vary.

I wouldn't say I personally hate it. Event bus design pattern has it's place. I think that - as usual with engineering concepts - some people observed abuses and spoke loud about them. Some were more mature at verbalizing the issues, some - maybe pursuing social maedia traction - just vented out anger calling ppl names.

If we ask ourselves "why" in a serious manner, I can find few issues with event bus pattern in general, in no relation to Nextcloud app:

  1. it's very temping to inroduce hidden tight coupling between componets (no direct source-level dependency, but dependencies caused by complex event patterns)
  2. cascade of event posts makes callstack analysis challenging; it becomes very tricky if events are sent bi-directionally between many actors; this is what makes contribues seriously to microservices debugging difficulty and market is raging with fancy distributed-log-as-a-service solutions
  3. event storms :-)
  4. upredictable and untraceable code paths - your component can be called from many places

Those issues don't necessarily make event bus an anti-pattern, but migh influence a decision of using it or not. It can surely save you some typing, but typing is not a bottleneck in more complex systems. Also, at certain level of complexity, team/company social considerations cannot be dismissed lightly and more involving patterns, but requiring less engineering discipline might be preferred over more convenient ones, but requiring more informed or governed approach.

BTW, we have LocalBroadcastManager that can serve this role as well. The drawback of LMB is that it requires Android platform in tests, so I wouldn't be so sure at all that we are anxious to remove EventBus. :-)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am aware of "people" complaining and the bad patterns surrounding EventBus, some of which are also inside Nextcloud (not the least, some written by me). Nevertheless, I really appreciate you taking the time to explain your thoughts in detail. Thank you once again for being awesome!

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
@tobiasKaminsky

Copy link
Copy Markdown
Member

@ezaquarii I took the liberty to change the last cancelContactBackupJobForAccount to use also User
7b4e1b8

Otherwis fine and approved 🎉

@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

Codacy Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 1
- Added 4
           

Complexity increasing per file
==============================
- src/main/java/com/owncloud/android/jobs/AccountRemovalJob.java  2
         

See the complete overview on Codacy

Comment thread src/main/java/com/owncloud/android/jobs/OfflineSyncJob.java
Comment thread src/main/java/com/owncloud/android/jobs/OfflineSyncJob.java
@ezaquarii

Copy link
Copy Markdown
Collaborator Author

@ezaquarii I took the liberty to change the last cancelContactBackupJobForAccount to use also User
7b4e1b8

Otherwis fine and approved tada

Thanks! I'm done here for now, so let's ship it.

@ezaquarii

Copy link
Copy Markdown
Collaborator Author

@tobiasKaminsky Drone hangs and it's not the first time it does it.

drone-hangs

Would it be possible to set a timeout or - even better - timeout + automatic restart?

@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

@tobiasKaminsky

Copy link
Copy Markdown
Member

Would it be possible to set a timeout or - even better - timeout + automatic restart?

There is a timeout after 30min, not sure why this sometimes is ignored.

@tobiasKaminsky

Copy link
Copy Markdown
Member

I restarted, and then merge :-)

@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

APK file: https://www.kaminsky.me/nc-dev/android-artifacts/12709.apk

qrcode

To test this change/fix you can simply download above APK file and install and test it in parallel to your existing Nextcloud app.

@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

Codacy

347

Lint

TypemasterPR
Warnings7777
Errors00

SpotBugs (new)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings71
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings78
Security Warnings45
Dodgy code Warnings139
Total386

SpotBugs (master)

Warning TypeNumber
Bad practice Warnings26
Correctness Warnings71
Internationalization Warnings13
Malicious code vulnerability Warnings5
Multithreaded correctness Warnings9
Performance Warnings78
Security Warnings45
Dodgy code Warnings139
Total386

@nextcloud-android-bot

Copy link
Copy Markdown
Collaborator

@tobiasKaminsky
tobiasKaminsky merged commit 4616b0d into master Feb 17, 2020
@delete-merged-branch
delete-merged-branch Bot deleted the ezaquarii/migrate-jobs-to-new-user-model branch February 17, 2020 09:40
@AndyScherzinger AndyScherzinger added this to the Nextcloud App 3.11.0 milestone Feb 17, 2020
tobiasKaminsky added a commit that referenced this pull request Feb 18, 2020
2b26a51 Merge remote-tracking branch 'origin/master' into dev
4616b0d Merge pull request #5447 from nextcloud/ezaquarii/migrate-jobs-to-new-user-model
20b910a Merge pull request #5442 from nextcloud/ezaquarii/fix-crash-on-text-file-creation
0977c37 Fix crash during text file creation
5c74f2b [tx-robot] updated from transifex
27fabe7 [tx-robot] updated from transifex
6029548 daily dev 20200215
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants